Skip to content

ffi: validate 'void' as parameter type in getFunction and getFunctions#63504

Open
Anshikakalpana wants to merge 1 commit into
nodejs:mainfrom
Anshikakalpana:ffi-void-parameter
Open

ffi: validate 'void' as parameter type in getFunction and getFunctions#63504
Anshikakalpana wants to merge 1 commit into
nodejs:mainfrom
Anshikakalpana:ffi-void-parameter

Conversation

@Anshikakalpana
Copy link
Copy Markdown
Contributor

@Anshikakalpana Anshikakalpana commented May 23, 2026

Fixes: #63461

Passing 'void' as a parameter type in getFunction or getFunctions triggers an internal assertion instead of a user-friendly error.

In C, void in a parameter list means a function takes no arguments, expressed in Node.js FFI as an empty array []. There is no valid use case for 'void' as a parameter type.

Fix:
add early validation in getFunction and getFunctions that throws ERR_INVALID_ARG_VALUE when 'void' appears in the parameters array, before wrapWithSharedBuffer is called.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 23, 2026
@Anshikakalpana Anshikakalpana mentioned this pull request May 23, 2026
Open
@ShogunPanda
Copy link
Copy Markdown
Contributor

ShogunPanda commented May 23, 2026

No, this is wrong. Don't validate at JS level but within C++ level please.

@Anshikakalpana Anshikakalpana force-pushed the ffi-void-parameter branch 2 times, most recently from 455f25e to 06410ff Compare May 23, 2026 10:19
@Anshikakalpana
Copy link
Copy Markdown
Contributor Author

Anshikakalpana commented May 23, 2026

No, this is wrong. Don't validate at JS level but within C++ level please.

Moved validation into the native FFI signature parsing layer in src/ffi/types.cc as suggested. Also removed the temporary JS-side validation.

ShogunPanda
ShogunPanda approved these changes May 23, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 23, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 23, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/73625/

@codecov
Copy link
Copy Markdown

codecov Bot commented May 23, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.33%. Comparing base (2e3daf6) to head (78160bf).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #63504      +/-   ##
==========================================
- Coverage   90.34%   90.33%   -0.01%     
==========================================
  Files         730      730              
  Lines      234359   234367       +8     
  Branches    43923    43917       -6     
==========================================
+ Hits       211720   211723       +3     
- Misses      14361    14373      +12     
+ Partials     8278     8271       -7
Files with missing lines Coverage Δ
src/ffi/types.cc 47.51% <100.00%> (+0.60%) ⬆️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ShogunPanda
Copy link
Copy Markdown
Contributor

ShogunPanda commented May 24, 2026

CI failures are relevant

@Anshikakalpana
Copy link
Copy Markdown
Contributor Author

Anshikakalpana commented May 24, 2026 •
edited
Loading

CI failures are relevant

Hi @ShogunPanda, the Jenkins failures are from the old commit 06410ff (confirmed via COMMIT_SHA_CHECK in the job parameters). GitHub Actions on the current commit 618bed6 are fully green. Could you please trigger a fresh CI run?

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 24, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/73650/

@Anshikakalpana
Copy link
Copy Markdown
Contributor Author

Anshikakalpana commented May 24, 2026

Fixed the Windows failure — DynamicLibrary(null) rejects null on Windows. Now using kernel32.dll on Windows and null on Unix. Sorry for the multiple runs!

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 24, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/73656/

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 25, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/73688/

@ShogunPanda
Copy link
Copy Markdown
Contributor

ShogunPanda commented May 25, 2026

@Anshikakalpana unfortunately the failure still seems relevant. Can you please take a look?

@Anshikakalpana Anshikakalpana force-pushed the ffi-void-parameter branch 2 times, most recently from a28da0d to 5683e58 Compare May 25, 2026 07:42
@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 25, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/73701/

@ShogunPanda
Copy link
Copy Markdown
Contributor

ShogunPanda commented May 25, 2026

Still failing :(

@Anshikakalpana
ffi: validate 'void' as parameter type in getFunction and getFunctions
78160bf 
Fixes: nodejs#63461
Signed-off-by: Anshikakalpana <anshikajain196872@gmail.com>
@Anshikakalpana
Copy link
Copy Markdown
Contributor Author

Anshikakalpana commented May 25, 2026

Update: found the issue — assert.throws with a regex matches against the error message, not the code. Fixed both assertions to use { code: 'ERR_INVALID_ARG_VALUE' }. Latest push should be green.

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 25, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/73709/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShogunPanda ShogunPanda ShogunPanda approved these changes

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ffi: 'void' as parameter type triggers internal assertion

3 participants

@Anshikakalpana @ShogunPanda @nodejs-github-bot